feat: new check editor #1280
feat: new check editor #1280
Conversation
- dump code on to fresh `main` branch - add feature flag `synthetic-monitoring-check-editor` (`FeatureName.CheckEditor`) - wire up routes (edit/new) - create submit handler hook - create after save nav hook for checks - add new datasource method
- remove `console.log` (except one)
- add RegExp support for checking "known" log messages
Script size changes
Totals
|
- fix eslint import error
- handle navigation on error (wip)
- remove console.log
- Remove unused files - restructure/rename components
- add "goto error" functionallity - setup all checks/sections with error fields
- fix bug that would case app to explode if invalid checktype was supplied
- enable `<ConfirmLeavingPage />` - remove unused code - update README.md
- add feature concept
- fix feature concept panic render
- add secrets to right aside
- fix word-break on log `msg`
- `adhoc-check`: handle use of `expect` - move components to own files - handle submit and submitting better
- handle form errors for adhoc checks
- fix: handle form errors for adhoc checks
- handle custom section labels - better handling of existing checks (edit)
- handle custom section labels - better handling of existing checks (edit) - add tests
- handle custom section labels - better handling of existing checks (edit) - add tests
- handle custom section labels - better handling of existing checks (edit) - add tests
- handle custom section labels - better handling of existing checks (edit) - add tests
- add more tests
- fix active label bug
- add label limits to label section
Screen.Recording.2025-10-15.at.10.26.44.movFound the check type selection automatically validates the form when I first arrive on it. It also doesn't update the query parameter in the URL. 
Grot also looks a little unwell to me! |
Screen.Recording.2025-10-15.at.10.38.04.movAlso found that for MultiStep for query parameters it didn't go to the right tab with the error. Screen.Recording.2025-10-15.at.10.40.13.movWhen the error was in the variable dropdown:
Screen.Recording.2025-10-15.at.10.41.50.movIn MultiStep when I add several assertions, it doesn't scroll down to the one with the error either. |
Screen.Recording.2025-10-15.at.10.45.30.movBit of a smaller UX issue which I don't mind if we tackle after this PR but when I'm using my laptop with it zoomed in (16" MacBook Pro with 125% zoom), the editor starts to feel pretty claustrophobic -- especially when doing scripted / browser checks. I know I was complaining about the double scrollbar and the non-visible save button at times so not sure what the easy fix is here. Maybe a toggle or some such to expand the editor? Probably a larger consideration to tackle so let's work on this afterwards. With that said, I wonder if a quick win is to get rid of the section title for a bit of extra real estate? We know what section we are in via the wizard form steps. 
|
- fix: form skipping all steps on check type change
- fix: form skipping all steps on check type change - fix: FormTabs not activating error tab unless already mounted - fix: multi http add query param button text - fix: variables not expanding on error - fix: variables not registered as a section error
|
✅ Probe API Server Mappings Verification Passed The |
- import from CheckEditor instead of duplicating
- import from CheckEditor instead of duplicating (continuation)
- refactor: hide section `h2` by default
|
Awesome work! 👏 👏 👏 Some small things I saw:
|
- fix: `onCheckTypeChange` prop for handling search param updates - chore: `yarn.lock` - fix: no trigger validation blindly, on check type change - fix: the feedback tooltip is positioned incorrectly
Caused by a bug in floating-ui (see floating-ui/floating-ui#3067).
"Query parameters" (protocol checks) is just a helper to add/modify the URL. The URL itself has validation. Adding a bunch of empty search params is not "invalid", but it's not so nice looking. When it comes to empty values, the only difference is that the old editor didnt write the empty values until you wrote something in at least on of the values (other than that, it also supports empty values). If we want to block the user from creating empty search params, why do we want to do it?
Totally agree (so does @ckbedwell), persisting adhoc data when switching tab is totally what we want to do. Just not in this PR.
Having too much spacing/gap makes it look a bit strange (because of the gap between description and input). I added some extra gap between the input fields, and a bigger gab between the last input and the "add" button. 
|
There was a problem hiding this comment.
Alright, the only show stopper for me is that most of the integration testing suite is not hooked up. That is the only way I am going to have confidence that every variation of submitting the checks is catered for as last time we did a check editor refactor we missed a bunch of stuff and that testing suite was written as a solution to cater for that in the future. Some of those tests won't work as they are because some of the tabs have been consolidated (I saw the DNS request tabs were consolidated -- which makes sense but should have been a separate PR...) and some of the other components in the Uptime stage have been rewritten.
So once the Checkster is running the tests in src/page/NewCheck/__tests__/ and src/page/EditCheck/__tests__/ I'll happily approve.
I've left a smattering of comments across the whole PR. It's up to you to decide how you want to address them in this PR or in follow up PRs. I think the one thing that stood out to me is a bit of a lack of standardisation for the styling.
- There was a lot of inline emotion css calls which are just noisy and distracting
- There were some files where the styles were gotten from a function called
getClassName-- which despite being more technically correct, for whatever reason the convention at Grafana seems to have settled ongetStyles. I don't mind which we choose I just want it to be consistent.
Lastly, I could see a lot of fighting with @grafana/ui in this PR and I'd like to understand the recreation of some base components further with some comments or documentation.
I would have much rathered we accepted its limitations of the UI package rather than scope-creeped with entirely new components that should really have been reviewed and assessed individually but we can take that on as tech debt and circle back.
src/components/Checkster/components/CollapsibleRequestEntry.tsx
Outdated
Show resolved
Hide resolved
| } | ||
| // Style by proxy | ||
| & > h4 { |
There was a problem hiding this comment.
😃 🔫
Raised my eyebrows at this, reread the code then realised it was just a rehash of the conversation we had this morning 🤦
I'm putting that eslint rule in because being forced to do this kind of thing is so dumb.
| const allErrors = useSilentErrors(); | ||
|
|
||
| return ( | ||
| <ol aria-label="Check form navigation" role="navigation" className={styles.container}> |
There was a problem hiding this comment.
Because they aren't links this wouldn't be navigation but probably a tablist?
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/tablist_role
There was a problem hiding this comment.
Had to change a lot of stuff for the tablist 😩
Please take a look at the tablist, tab and tabpanel roles associated with the nav.
| mockGetCheckType.mockReturnValue(CheckType.DNS); | ||
| mockGetDNSCheckFormValues.mockReturnValue(mockFormValues); | ||
|
|
||
| const result = toFormValues(mockCheck); | ||
|
|
||
| expect(mockGetCheckType).toHaveBeenCalledWith(mockCheck.settings); | ||
| expect(mockGetDNSCheckFormValues).toHaveBeenCalledWith(mockCheck); | ||
| expect(result).toBe(mockFormValues); |
There was a problem hiding this comment.
Honestly, I see very little value in these tests due to the heavy use of mocking and type casting. It's almost 500 lines which is essentially checking if the switch statements work but not the actual transformations themselves. My expectation when reading this file was that it would be testing the transformations for each check type.
In the first half of the tests every test returns the HTTP check type in the result so it's difficult to have confidence in a test that is focussed on DNS when the input and result has nothing to do with DNS. If you were to keep these tests I'd make it much clearer they are just testing the switch statements and looking at parameterising them so it's just a for loop that is lining up checkType with the transformation function.
There was a problem hiding this comment.
It's almost 500 lines which is essentially checking if the switch statements work
That is also exactly what the module does. But I agree, it's not pretty and it can definitely be improved.
Ideally each individual toPayload. and toFormValues. should have their own unit tests and not be tested via adaptors.tests.ts.
- fix: add some additional spacing to name/value fields
- cr: add todo for eventually unused prop - cr: combined prop difficulties - cr: `open` prop to `isOpen` - cr: this wouldn't be navigation but probably a `tablist` - cr: onfusing that the component changes to a Fragment but still receives props - cr: remove unused styles - cr: use `iconPlacement` instead of constructing it manually - cr: move utils function to *.utils.ts (query params) - fix: add tabIndex - fix: InputSelect.tsx focus styling - cr: refactor `handleSubmit` - cr: refactor inline `css` - fix: spelling "Reuqest target" - fix: TracerouteCheckContent input type and options fields - cr: remove check type modal - cr: consolidate `ValidationError` and `ValidationWarning`
- cr: use single point of truth for example k6 scripts - cr: use constants instead of math as frequency/timeout - cr: update FeatureTabContext types - cr: fix hardcoded colors (use theme colors) - cr: remove unused code - cr: rename `useSilentErrors` - cr: remove incomplete tests - cr: remove helper class for bodySmall - cr: move styles (delete styles.ts) - cr: rename getDefaultCheckConfig
- fix: handle adhoc checks with no logs
- chore: fix typings after last commit - fix: multihttp entries buried errors
| </StyledField> | ||
| <Text variant="bodySmall" color="secondary"> | ||
| Looking for <Text color="primary">"Cache busting query parameter"</Text>? It can be found under | ||
| <Text color="primary">"Request options"</Text>; |
There was a problem hiding this comment.
| <Text color="primary">"Request options"</Text>; | |
| <Text color="primary">"Request options"</Text> |
- fix: overlimits - fix: read logs checking - fix: missing disabled - chore: add updated test utils - chore: add testids for Checkster - chore: add broken test (`1-request.ui.test.tsx`)
- chore: add tests `gRPC`
- chore: add tests `ping`
3 participants
Reworked check editor for synthetic checks
Complete re-creation of the current check Form.
Why? As we require more features and customization of the Check form, it has become increasingly complex to do the simplest changes. In an attempt to simplify and surface inner workings, "everything" has been remade.
The name "Checkster" is a placeholder name that will later be refactored into "CheckEditor", as soon as we have removed the old "CheckEditor".
Features
This PR also brings new features to the check authoring view:
How to test
synthetic-monitoring-check-editorAdhoc check example (blackbox exporter)
Adhoc check example (k6/multihttp)
Adhoc check example (k6)
Additional context
Resolves #1263
Resolves #1174
Resolves #953
Resolves #1133